Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split tc programs from generic tracer #1267

Merged
merged 7 commits into from
Oct 18, 2024
Merged

Split tc programs from generic tracer #1267

merged 7 commits into from
Oct 18, 2024

Conversation

rafaelroquetto
Copy link
Contributor

@rafaelroquetto rafaelroquetto commented Oct 17, 2024

This PR splits the "tc" ebpf programs off generictracer. This allows for the tc programs to be orthogonally loaded, and for more fine grained permissions.

In a nutshell, this PR entails:

  • removing code duplication in the ebpf loader
  • splitting a few ebpf maps to their own files, to reduce unecessary dependency and code generation
  • moving the tc programs to their own newly created tracer called tc_tracer
  • ensure threads be properly joined during program shutdown, allowing cleanup goroutines to complete gracefully (fixes a bug in which we left qdiscs behind, among other stuff)
  • explicitly test for CAP_NET_ADMIN, which is only required when BEYLA_BPF_TC=1.

This PR is easier reviewed on a per-commit basis

@@ -98,5 +98,9 @@ func newGoTracersGroup(cfg *beyla.Config, metrics imetrics.Reporter) []ebpf.Trac
}

func newGenericTracersGroup(cfg *beyla.Config, metrics imetrics.Reporter) []ebpf.Tracer {
return []ebpf.Tracer{generictracer.New(cfg, metrics), tctracer.New(cfg)}
if cfg.EBPF.UseLinuxTC {
return []ebpf.Tracer{generictracer.New(cfg, metrics), tctracer.New(cfg)}
Copy link
Contributor Author

@rafaelroquetto rafaelroquetto Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be done here, because simply not calling tctracer.SetupTC() is not enough. The mere attempt to load the ebpf program (as in ebpf.LoadAndAssign()), which happens way before SetupTC() is called, will trigger the kernel verifier in case CAP_NET_ADMIN is not present (which may be a legitmate case when UseLinuxTC is false).

This also keeps the logic giving it visibility where it belongs IMHO.

@rafaelroquetto rafaelroquetto marked this pull request as ready for review October 17, 2024 20:19
@rafaelroquetto
Copy link
Contributor Author

The original generic_tracer tests comprised just a test for the pid segment bitmap handling functions. It is difficult to test the tracer without privileges, as such test would basically test whether the tracer is being loaded into the kernel and then unloaded.

If such a test is nonetheless desired, that can perhaps be worked out by adding one of the _test_privilleged.go files.

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 16.78322% with 238 lines in your changes missing coverage. Please review.

Project coverage is 80.48%. Comparing base (302d88a) to head (538f800).

Files with missing lines Patch % Lines
pkg/internal/ebpf/tctracer/tctracer.go 0.00% 199 Missing ⚠️
pkg/internal/ebpf/tracer_linux.go 50.00% 24 Missing and 8 partials ⚠️
pkg/beyla/os.go 25.00% 2 Missing and 1 partial ⚠️
pkg/internal/discover/finder.go 0.00% 2 Missing and 1 partial ⚠️
pkg/components/beyla.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1267      +/-   ##
==========================================
- Coverage   80.77%   80.48%   -0.30%     
==========================================
  Files         131      132       +1     
  Lines       13465    13535      +70     
==========================================
+ Hits        10876    10893      +17     
- Misses       2079     2127      +48     
- Partials      510      515       +5     
Flag Coverage Δ
integration-test 60.97% <16.43%> (-0.17%) ⬇️
k8s-integration-test 57.26% <16.43%> (-0.02%) ⬇️
oats-test 36.39% <16.43%> (-0.01%) ⬇️
unittests 53.04% <0.34%> (-0.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rafaelroquetto rafaelroquetto changed the title WIP: Split tc programs from generic tracer Split tc programs from generic tracer Oct 17, 2024
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice work @rafaelroquetto !

@rafaelroquetto rafaelroquetto merged commit ca5f23e into main Oct 18, 2024
12 checks passed
@rafaelroquetto rafaelroquetto deleted the tc_split branch October 18, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants